feat: adding kustomizations via UI #387
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds the ability to create Flux Kustomizations through the UI, along with enhanced navigation between related resources. The implementation includes a new dialog component, custom hooks for creation and navigation, and improved cross-referencing between Kustomizations, Git Repositories, and Managed Resources.
- New CreateKustomizationDialog component with form validation and substitution support
- Tab-based navigation with URL state management for better user experience
- Cross-linking between resources (Kustomizations → Git Repositories, Managed Resources → Kustomizations)
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/spaces/mcp/pages/McpPage.tsx | Added URL-based tab navigation with search params to maintain state across page navigation |
| src/lib/api/types/flux/createKustomization.ts | Type definition for Kustomization creation payload |
| src/hooks/useNavigateToTab.ts | Reusable hook for navigating to tabs with hash support |
| src/hooks/useNavigateToTab.spec.ts | Unit tests for the navigation hook |
| src/hooks/useCreateKustomization.ts | Hook for creating Kustomizations with API integration |
| src/hooks/useCreateKustomization.spec.ts | Comprehensive unit tests for the creation hook |
| src/components/Dialogs/CreateKustomizationDialog.tsx | Form dialog for creating new Kustomizations with validation |
| src/components/Dialogs/CreateKustomizationDialog.module.css | Styling for the dialog component |
| src/components/Dialogs/CreateKustomizationDialog.cy.tsx | Component tests for the dialog |
| src/components/ControlPlane/ManagedResources.tsx | Added "Managed By" column linking to Kustomizations |
| src/components/ControlPlane/Kustomizations.tsx | Added create button, source reference links, and scroll-to navigation |
| src/components/ControlPlane/GitRepositories.tsx | Added URL display and ID anchors for scroll-to functionality |
| public/locales/en.json | Added localization strings for new dialog and features |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| setTimeout(() => { | ||
| const element = document.getElementById(hash); | ||
| if (element) { | ||
| element.scrollIntoView({ behavior: 'smooth', block: 'center' }); | ||
| } | ||
| }, 500); |
There was a problem hiding this comment.
[nitpick] The scroll behavior uses a hardcoded 500ms timeout. This magic number should be extracted as a named constant to improve code maintainability and make it easier to adjust if needed.
| useEffect(() => { | ||
| if (!isLoading && rows.length > 0 && location.hash) { | ||
| const hash = location.hash.substring(1); | ||
| if (hash.startsWith('kustomization-')) { | ||
| const targetName = hash.replace('kustomization-', ''); | ||
| const index = rows.findIndex((row) => row.name === targetName); | ||
|
|
||
| if (index !== -1) { | ||
| setTimeout(() => { | ||
| const element = document.getElementById(hash); | ||
| if (element) { | ||
| element.scrollIntoView({ behavior: 'smooth', block: 'center' }); | ||
| } | ||
| }, 500); | ||
| } | ||
| } | ||
| } | ||
| }, [isLoading, rows, location.hash]); |
There was a problem hiding this comment.
The scroll behavior implementation queries the DOM using document.getElementById which couples the component to the DOM structure. Additionally, there's no cleanup if the component unmounts during the timeout. Consider using a ref-based approach or adding cleanup logic to cancel the timeout.
| <Link | ||
| onClick={() => { | ||
| const element = document.getElementById(`git-repository-${value}`); | ||
| if (element) { | ||
| element.scrollIntoView({ behavior: 'smooth', block: 'center' }); | ||
| } | ||
| }} | ||
| > | ||
| {value} | ||
| </Link> | ||
| ), |
There was a problem hiding this comment.
[nitpick] The scroll-to behavior in the Link's onClick handler uses document.getElementById and scrollIntoView directly. This is duplicated with the useEffect logic (lines 199-216). Consider extracting this into a reusable helper function to avoid code duplication.
| { | ||
| Header: t('ManagedResources.tableHeaderManagedBy'), | ||
| accessor: (row: ResourceRow) => | ||
| (row.item.metadata?.labels as unknown as Record<string, string> | undefined)?.[ | ||
| 'kustomize.toolkit.fluxcd.io/name' | ||
| ], | ||
| Cell: ({ cell: { value } }) => | ||
| value ? ( | ||
| <Link | ||
| onClick={() => { | ||
| navigateToTab('flux', `kustomization-${value}`); | ||
| }} | ||
| > | ||
| {value} | ||
| </Link> | ||
| ) : null, | ||
| }, |
There was a problem hiding this comment.
The new "Managed By" column added to ManagedResources lacks test coverage. Since ManagedResources.cy.tsx exists with comprehensive tests, consider adding test cases to verify the new column's behavior, including the link navigation functionality.
| </ObjectPageHeader> | ||
| } | ||
| onSelectedSectionChange={() => setSelectedSectionId(undefined)} | ||
| onSelectedSectionChange={handleSectionChange} |
There was a problem hiding this comment.
The onSelectedSectionChange handler was previously set to clear the selected section (setting it to undefined). The new implementation updates both state and URL params, but this changes the previous behavior. Verify that removing the section clearing logic doesn't break existing functionality.
| accessor: 'created', | ||
| }, | ||
| { | ||
| Header: t('ManagedResources.tableHeaderManagedBy'), |
There was a problem hiding this comment.
The new "Managed By" column uses a translation key 'ManagedResources.tableHeaderManagedBy' but this key is not defined in the localization file (public/locales/en.json). This will result in the translation key being displayed instead of the proper text. Add the missing translation key.
| // Effect to handle tab switching via URL params | ||
| useEffect(() => { | ||
| const tab = searchParams.get('tab'); | ||
| if (tab && ['overview', 'crossplane', 'flux', 'landscapers'].includes(tab)) { |
There was a problem hiding this comment.
The tab validation hardcodes the allowed tab values in a string array. Consider extracting this as a constant or type guard to ensure consistency with the McpPageSectionId type and avoid duplication.
| onNavigateToMcpSection={(sectionId) => { | ||
| setSelectedSectionId(sectionId); | ||
| setSearchParams((prev) => { | ||
| const newParams = new URLSearchParams(prev); | ||
| newParams.set('tab', sectionId); | ||
| return newParams; | ||
| }); | ||
| }} |
There was a problem hiding this comment.
There's code duplication between lines 172-179 and the handleSectionChange function (lines 83-93). The logic for updating search params with the tab is repeated. Consider extracting this into a shared helper function or reusing handleSectionChange.
| const targetNamespace = data.namespace || defaultNamespace; | ||
|
|
||
| const substitutionsMap: Record<string, string> = {}; | ||
| data.substitutions?.forEach((sub) => { | ||
| if (sub.key) substitutionsMap[sub.key] = sub.value; | ||
| }); | ||
|
|
||
| const payload: CreateKustomizationType = { | ||
| apiVersion: 'kustomize.toolkit.fluxcd.io/v1', | ||
| kind: 'Kustomization', | ||
| metadata: { | ||
| name: data.name, | ||
| namespace: targetNamespace, | ||
| }, | ||
| spec: { | ||
| interval: data.interval, | ||
| sourceRef: { | ||
| kind: 'GitRepository', | ||
| name: data.sourceRefName, | ||
| }, | ||
| path: data.path, | ||
| prune: data.prune, | ||
| targetNamespace: data.targetNamespace || undefined, |
There was a problem hiding this comment.
[nitpick] The targetNamespace field is set to either the provided value or undefined. However, line 32 already extracts data.namespace which might be confused with data.targetNamespace. The naming could be clearer - targetNamespace in the metadata defines where the Kustomization resource lives, while spec.targetNamespace defines where resources are deployed. Consider adding a comment to clarify this distinction.
| substitutions: z | ||
| .array( | ||
| z.object({ | ||
| key: z.string().min(1, { message: t('validationErrors.required') }), | ||
| value: z.string().min(1, { message: t('validationErrors.required') }), | ||
| }), |
There was a problem hiding this comment.
The substitutions validation requires both key and value to have minimum length of 1, but in the hook implementation (useCreateKustomization.ts line 36), only the key is checked before adding to the map. This creates an inconsistency between validation and actual behavior.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer: